Revert 19658:28a197617286 "Fix up the synchronisation around grant

table map track handles".

There is no race since the hypercall takes the
domain-lock. Furthermore removing locking from get_maptrack_handle()
races gnttab_setup_table().

Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 58011836dbf99ecad654ed6625fc13d5fa781a83..7bbc05d896724a5cdf9680a9631e26ffcfc1e7e2 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -111,26 +111,6 @@ static unsigned inline int max_nr_maptrack_frames(void)
 #define active_entry(t, e) \
     ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])
 
-/* Technically, we only really need to acquire the lock for SMP
-   guests, because you only ever touch the maptrack tables from the
-   context of the guest which owns them, so if it's uniproc then the
-   lock can't be contended, and is therefore pointless.  Don't bother
-   with that optimisation for now, though, because it's scary and
-   confusing. */
-/* The maptrack lock is top-level: you're not allowed to be holding
-   any other locks when you acquire it. */
-static void
-maptrack_lock(struct grant_table *lgt)
-{
-    spin_lock(&lgt->maptrack_lock);
-}
-
-static void
-maptrack_unlock(struct grant_table *lgt)
-{
-    spin_unlock(&lgt->maptrack_lock);
-}
-
 static inline int
 __get_maptrack_handle(
     struct grant_table *t)
@@ -161,30 +141,43 @@ get_maptrack_handle(
 
     if ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
     {
-        nr_frames = nr_maptrack_frames(lgt);
-        if ( nr_frames >= max_nr_maptrack_frames() )
-            return -1;
+        spin_lock(&lgt->lock);
 
-        new_mt = alloc_xenheap_page();
-        if ( new_mt == NULL )
-            return -1;
+        if ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
+        {
+            nr_frames = nr_maptrack_frames(lgt);
+            if ( nr_frames >= max_nr_maptrack_frames() )
+            {
+                spin_unlock(&lgt->lock);
+                return -1;
+            }
 
-        clear_page(new_mt);
+            new_mt = alloc_xenheap_page();
+            if ( new_mt == NULL )
+            {
+                spin_unlock(&lgt->lock);
+                return -1;
+            }
 
-        new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
+            clear_page(new_mt);
 
-        for ( i = lgt->maptrack_limit; i < new_mt_limit; i++ )
-        {
-            new_mt[i % MAPTRACK_PER_PAGE].ref = i+1;
-            new_mt[i % MAPTRACK_PER_PAGE].flags = 0;
-        }
+            new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
+
+            for ( i = lgt->maptrack_limit; i < new_mt_limit; i++ )
+            {
+                new_mt[i % MAPTRACK_PER_PAGE].ref = i+1;
+                new_mt[i % MAPTRACK_PER_PAGE].flags = 0;
+            }
 
-        lgt->maptrack[nr_frames] = new_mt;
-        lgt->maptrack_limit      = new_mt_limit;
+            lgt->maptrack[nr_frames] = new_mt;
+            lgt->maptrack_limit      = new_mt_limit;
 
-        gdprintk(XENLOG_INFO,
-                 "Increased maptrack size to %u frames.\n", nr_frames + 1);
-        handle = __get_maptrack_handle(lgt);
+            gdprintk(XENLOG_INFO,
+                    "Increased maptrack size to %u frames.\n", nr_frames + 1);
+            handle = __get_maptrack_handle(lgt);
+        }
+
+        spin_unlock(&lgt->lock);
     }
     return handle;
 }
@@ -1515,9 +1508,7 @@ do_grant_table_op(
             guest_handle_cast(uop, gnttab_map_grant_ref_t);
         if ( unlikely(!guest_handle_okay(map, count)) )
             goto out;
-        maptrack_lock(current->domain->grant_table);
         rc = gnttab_map_grant_ref(map, count);
-        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_unmap_grant_ref:
@@ -1526,9 +1517,7 @@ do_grant_table_op(
             guest_handle_cast(uop, gnttab_unmap_grant_ref_t);
         if ( unlikely(!guest_handle_okay(unmap, count)) )
             goto out;
-        maptrack_lock(current->domain->grant_table);
         rc = gnttab_unmap_grant_ref(unmap, count);
-        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_unmap_and_replace:
@@ -1540,9 +1529,7 @@ do_grant_table_op(
         rc = -ENOSYS;
         if ( unlikely(!replace_grant_supported()) )
             goto out;
-        maptrack_lock(current->domain->grant_table);
         rc = gnttab_unmap_and_replace(unmap, count);
-        maptrack_unlock(current->domain->grant_table);
         break;
     }
     case GNTTABOP_setup_table:
@@ -1613,7 +1600,6 @@ grant_table_create(
     /* Simple stuff. */
     memset(t, 0, sizeof(*t));
     spin_lock_init(&t->lock);
-    spin_lock_init(&t->maptrack_lock);
     t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
 
     /* Active grant table. */
@@ -1694,7 +1680,6 @@ gnttab_release_mappings(
 
     for ( handle = 0; handle < gt->maptrack_limit; handle++ )
     {
-        /* Domain is dying, so don't need maptrack lock */
         map = &maptrack_entry(gt, handle);
         if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) )
             continue;

diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 1ab59434ab327ad18bdd2d6aaeaa68d144f07217..096af9bb4cad179d8d9b20077acd81cd56d7f3f3 100644 (file)
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -91,8 +91,6 @@ struct grant_table {
     struct grant_mapping **maptrack;
     unsigned int          maptrack_head;
     unsigned int          maptrack_limit;
-    /* Lock protecting maptrack-related fields. */
-    spinlock_t            maptrack_lock;
     /* Lock protecting updates to active and shared grant tables. */
     spinlock_t            lock;
 };